-
-
Notifications
You must be signed in to change notification settings - Fork 718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: simplified error handling in build-dashboard.js #3521
base: master
Are you sure you want to change the base?
fix: simplified error handling in build-dashboard.js #3521
Conversation
WalkthroughThe changes focus on modifying error handling in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3521--asyncapi-website.netlify.app/ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3521 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 19 20 +1
Lines 668 673 +5
=========================================
+ Hits 668 673 +5 ☔ View full report in Codecov by Sentry. |
@sambhavgupta0705 @devilkiller-ag Can you please review this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@devilkiller-ag Sorry Sir I requested for review again... It happened by mistake. |
@sahitya-chandra why are we having these changes? |
@sambhavgupta0705 Sir the code is returning a promise with error instead we can just throw an error |
I have just simplified the error handling in build-dashboard.js file as mentioned in #3305 issue |
we are using async/await so yes we can use this |
/update |
@sambhavgupta0705 @devilkiller-ag thanks for the review sir. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@anshgoyalevil can you please take a look at this |
This does solve the linked issue, but we are still looking for a better error handling strategy. Can we get #3358 done instead of just getting this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/tools/logger.js (1)
1-19
: Consider adjusting the formatting to meet Prettier style requirements.According to the static analysis hints, the file should avoid multiline bracket formatting in lines 5-8 and 11-14 and remove the trailing comma on line 16. Implementing these suggestions will keep the codebase consistent.
Here is a potential diff to adjust the formatting:
const logger = createLogger({ level: 'error', // minimum level - format: format.combine( - format.timestamp(), - format.json() - ), + format: format.combine(format.timestamp(), format.json()), transports: [ new transports.Console({ - format: format.combine( - format.colorize(), - format.simple() - ) + format: format.combine(format.colorize(), format.simple()) }) - ], + ] });🧰 Tools
🪛 eslint
[error] 5-8: Replace
⏎····format.timestamp(),⏎····format.json()⏎··
withformat.timestamp(),·format.json()
(prettier/prettier)
[error] 11-14: Replace
⏎········format.colorize(),⏎········format.simple()⏎······
withformat.colorize(),·format.simple()
(prettier/prettier)
[error] 16-16: Delete
,
(prettier/prettier)
[error] 19-19: Insert
⏎
(prettier/prettier)
tests/build-pages.test.js (2)
26-32
: Use arrow function parentheses for clarity.For the callback in
[TEST_DIR, NEW_TEST_DIR].forEach(dir => { ... })
, consider adding parentheses around the parameter for readability. This aligns with Prettier’s recommended styling.-[TEST_DIR, NEW_TEST_DIR].forEach(dir => { +[TEST_DIR, NEW_TEST_DIR].forEach((dir) => {🧰 Tools
🪛 eslint
[error] 26-27: Replace
⏎····[TEST_DIR,·NEW_TEST_DIR].forEach(dir
with····[TEST_DIR,·NEW_TEST_DIR].forEach((dir)
(prettier/prettier)
44-50
: Similarly apply arrow function parentheses.We can mirror the same style in the
afterAll
callback for consistency.🧰 Tools
🪛 eslint
[error] 44-45: Replace
⏎····[TEST_DIR,·NEW_TEST_DIR].forEach(dir
with····[TEST_DIR,·NEW_TEST_DIR].forEach((dir)
(prettier/prettier)
[error] 50-50: Delete
····
(prettier/prettier)
scripts/tools/tools-object.js (1)
11-11
: Replace ".js" extension for consistency.The static analysis suggests avoiding explicit file extensions. Using
'./logger'
instead of'./logger.js'
is an option if the codebase style allows it.-const logger = require("./logger.js"); +const logger = require("./logger");🧰 Tools
🪛 eslint
[error] 11-11: Replace
"./logger.js"
with'./logger.js'
(prettier/prettier)
[error] 11-11: Unexpected use of file extension "js" for "./logger.js"
(import/extensions)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json
(1 hunks)scripts/tools/logger.js
(1 hunks)scripts/tools/tools-object.js
(2 hunks)tests/build-pages.test.js
(3 hunks)
🧰 Additional context used
🪛 eslint
scripts/tools/logger.js
[error] 5-8: Replace ⏎····format.timestamp(),⏎····format.json()⏎··
with format.timestamp(),·format.json()
(prettier/prettier)
[error] 11-14: Replace ⏎········format.colorize(),⏎········format.simple()⏎······
with format.colorize(),·format.simple()
(prettier/prettier)
[error] 16-16: Delete ,
(prettier/prettier)
[error] 19-19: Insert ⏎
(prettier/prettier)
scripts/tools/tools-object.js
[error] 11-11: Replace "./logger.js"
with './logger.js'
(prettier/prettier)
[error] 11-11: Unexpected use of file extension "js" for "./logger.js"
(import/extensions)
[error] 111-111: Insert );⏎
(prettier/prettier)
[error] 117-117: Delete ,
(prettier/prettier)
tests/build-pages.test.js
[error] 26-27: Replace ⏎····[TEST_DIR,·NEW_TEST_DIR].forEach(dir
with ····[TEST_DIR,·NEW_TEST_DIR].forEach((dir)
(prettier/prettier)
[error] 44-45: Replace ⏎····[TEST_DIR,·NEW_TEST_DIR].forEach(dir
with ····[TEST_DIR,·NEW_TEST_DIR].forEach((dir)
(prettier/prettier)
[error] 50-50: Delete ····
(prettier/prettier)
[error] 65-66: Delete ⏎
(prettier/prettier)
🔇 Additional comments (4)
tests/build-pages.test.js (2)
23-23
: New constant introduced for directory handling.
Defining NEW_TEST_DIR
segregates it from TEST_DIR
, improving clarity in test directory setup. This looks good.
65-68
: Ensure directory is always cleaned up.
This test effectively verifies ensureDirectoryExists
. The explicit cleanup of NEW_TEST_DIR
is a good safety check. No issues here.
🧰 Tools
🪛 eslint
[error] 65-66: Delete ⏎
(prettier/prettier)
scripts/tools/tools-object.js (1)
114-118
: Structured logging is well-implemented.
Logging the message, stack, and phase helps debugging. This integration with Winston is properly done. Great job.
🧰 Tools
🪛 eslint
[error] 117-117: Delete ,
(prettier/prettier)
package.json (1)
118-118
: Dependency addition recognized.
The winston
library is widely used for logging. Including it here makes sense to support structured logging as implemented in scripts/tools/logger.js
.
@anshgoyalevil Please have a look sir... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/tools/logger.js (1)
3-11
: Consider enhancing logger configuration for better observabilityThe Winston logger setup implements good practices with structured logging (JSON format + timestamp). However, consider these enhancements:
- The 'error' level might miss important warnings. Consider using 'info' as the base level and filtering at the transport level.
- Add file transport for persistent logging in production.
const logger = createLogger({ - level: 'error', // minimum level of messages logger will handle + level: 'info', // capture all important logs format: format.combine(format.timestamp(), format.json()), transports: [ new transports.Console({ + level: 'error', // filter console output to errors format: format.combine(format.colorize(), format.simple()) - }) + }), + new transports.File({ + filename: 'error.log', + level: 'error', + format: format.combine( + format.timestamp(), + format.json() + ) + }) ] });scripts/tools/tools-object.js (2)
11-11
: Fix quote consistencyUse single quotes consistently across the file.
-const logger = require("./logger"); +const logger = require('./logger');🧰 Tools
🪛 eslint
[error] 11-11: Replace
"./logger"
with'./logger'
(prettier/prettier)
Line range hint
89-91
: Standardize error handling approachThe nested try-catch blocks have inconsistent error handling approaches. The inner catch throws directly while the outer catch logs and wraps. Consider standardizing the approach.
} catch (err) { - console.error(err) - throw err; + logger.error('Error processing individual tool', { + message: err.message, + stack: err.stack, + phase: 'individual_tool_processing', + }); + throw new Error(`Error processing individual tool: ${err.message}`); } })) return finalToolsObject; } catch (err) { logger.error('Error processing tool', { message: err.message, stack: err.stack, phase: 'tool_processing', }); throw new Error(`Error processing tool: ${err.message}`) }Also applies to: 111-120
🧰 Tools
🪛 eslint
[error] 8-8: Replace
"uri"])
with'uri']);
(prettier/prettier)
[error] 9-9: Insert
;
(prettier/prettier)
[error] 11-11: Replace
"./logger"
with'./logger'
(prettier/prettier)
tests/build-pages.test.js (1)
27-31
: Optimize test cleanup codeThe directory cleanup logic is duplicated in beforeAll and afterAll. Consider extracting it to a helper function.
+const cleanupTestDirs = () => { + [TEST_DIR, NEW_TEST_DIR].forEach((dir) => { + if (fs.existsSync(dir)) { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); +}; beforeAll(() => { - [TEST_DIR, NEW_TEST_DIR].forEach((dir) => { - if (fs.existsSync(dir)) { - fs.rmSync(dir, { recursive: true, force: true }); - } - }); + cleanupTestDirs(); // ... rest of setup }); afterAll(() => { - [TEST_DIR, NEW_TEST_DIR].forEach(dir => { - if (fs.existsSync(dir)) { - fs.rmSync(dir, { recursive: true, force: true }); - } - }); + cleanupTestDirs(); });Also applies to: 45-49
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/tools/logger.js
(1 hunks)scripts/tools/tools-object.js
(2 hunks)tests/build-pages.test.js
(3 hunks)
🧰 Additional context used
🪛 eslint
scripts/tools/logger.js
[error] 13-13: Insert ⏎
(prettier/prettier)
scripts/tools/tools-object.js
[error] 11-11: Replace "./logger"
with './logger'
(prettier/prettier)
[error] 111-111: Insert );⏎
(prettier/prettier)
[error] 117-117: Delete ,
(prettier/prettier)
tests/build-pages.test.js
[error] 26-27: Delete ⏎
(prettier/prettier)
[error] 44-45: Replace ⏎····[TEST_DIR,·NEW_TEST_DIR].forEach(dir
with ····[TEST_DIR,·NEW_TEST_DIR].forEach((dir)
(prettier/prettier)
[error] 50-50: Delete ····
(prettier/prettier)
[error] 65-66: Delete ⏎
(prettier/prettier)
🔇 Additional comments (1)
scripts/tools/tools-object.js (1)
114-118
: Good structured logging implementation
The error logging implementation provides good context with message, stack trace, and processing phase.
🧰 Tools
🪛 eslint
[error] 117-117: Delete ,
(prettier/prettier)
That's some good progress @sahitya-chandra But looking at the scope of this issue, it seems like we would have to move it to the bounty program as there are a lot of places where we have to strategically add Winston Logger, and that would also require a lot of script tests to be changed |
I think I can add winston to more places in the code in future |
@anshgoyalevil any updates sir... |
Description
Simplified the error handling in the build-dashboard.js file by replacing
return Promise.reject(e)
tothrow e
as suggested in PR fix: fixed dashboard scripts #3304added custom logservice using winston for error logging as mentioned in PR feat: add tests for tool-object script #3265
error logging img
build-pages.test.js
fileRelated issue(s)
resolved #3305 #3358
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests